notify extension dev server of app assets updates#7227
notify extension dev server of app assets updates#7227isaacroldan wants to merge 2 commits intomainfrom
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
2670bff to
7fd0331
Compare
1e0e9ea to
7efd3f7
Compare
73363b9 to
8e75a20
Compare
7efd3f7 to
85f5b0f
Compare
8e75a20 to
52ef686
Compare
85f5b0f to
4c6daca
Compare
52ef686 to
9cd7adc
Compare
4c6daca to
1ba2dd9
Compare
9cd7adc to
dc2a701
Compare
1ba2dd9 to
5dfacdb
Compare
dc2a701 to
c6ca3d1
Compare
82d0865 to
8c27933
Compare
665154d to
a1b4321
Compare
8c27933 to
fc760cc
Compare
a1b4321 to
fa4c046
Compare
3383e69 to
d1271b9
Compare
b655072 to
3dc00e6
Compare
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationsWe found no new type declarations in this PR Existing type declarationspackages/cli-kit/dist/private/node/ui.d.ts import { Logger, LogLevel } from '../../public/node/output.js';
-import React from 'react';
import { Key, RenderOptions } from 'ink';
import { EventEmitter } from 'events';
-/**
- * Signal that the current Ink tree is done. Must be called within an
- * InkLifecycleRoot — throws if the provider is missing so lifecycle
- * bugs surface immediately instead of silently hanging.
- */
-export declare function useComplete(): (error?: Error) => void;
-/**
- * Root wrapper for Ink trees. Owns the single `exit()` call site — children
- * signal completion via `useComplete()`, which sets state here. The `useEffect`
- * fires post-render, guaranteeing all batched state updates have been flushed
- * before the tree is torn down.
- */
-export declare function InkLifecycleRoot({ children }: {
- children: React.ReactNode;
-}): React.JSX.Element;
interface RenderOnceOptions {
logLevel?: LogLevel;
logger?: Logger;
renderOptions?: RenderOptions;
}
export declare function renderOnce(element: JSX.Element, { logLevel, renderOptions }: RenderOnceOptions): string | undefined;
-export declare function render(element: JSX.Element, options?: RenderOptions): Promise<void>;
+export declare function render(element: JSX.Element, options?: RenderOptions): Promise<unknown>;
export declare class Stdout extends EventEmitter {
columns: number;
rows: number;
readonly frames: string[];
private _lastFrame?;
constructor(options: {
columns?: number;
rows?: number;
});
write: (frame: string) => void;
lastFrame: () => string | undefined;
}
export declare function handleCtrlC(input: string, key: Key, exit?: () => void): void;
export {};
packages/cli-kit/dist/public/node/ui.d.ts@@ -34,7 +34,7 @@ export interface RenderConcurrentOptions extends PartialBy<ConcurrentOutputProps
* 00:00:00 │ frontend │ third frontend message
*
*/
-export declare function renderConcurrent({ renderOptions, ...props }: RenderConcurrentOptions): Promise<void>;
+export declare function renderConcurrent({ renderOptions, ...props }: RenderConcurrentOptions): Promise<unknown>;
export type AlertCustomSection = CustomSection;
export type RenderAlertOptions = Omit<AlertOptions, 'type'>;
/**
|
535fde3 to
5399272
Compare
d1271b9 to
06b34f4
Compare
5399272 to
42083ba
Compare
42083ba to
c77befd
Compare
There was a problem hiding this comment.
Pull request overview
This PR extends the local UI extensions development server to support serving app-level assets (notably admin static_root) and to propagate related metadata (URLs + timestamps) through the extensions payload so clients can fetch and refresh those assets during development.
Changes:
- Adds
app.allowed_domainsandapp.assetsmetadata into the extensions payload and shared type definitions. - Introduces an HTTP middleware + route (
/extensions/assets/:assetKey/...) for serving app assets from configured directories. - Expands dev file watching and build/include-assets behavior (watch
realExtensions; overwrite generatedmanifest.json; admin spec updates).
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ui-extensions-server-kit/src/types.ts | Extends App payload types with allowed_domains and assets. |
| packages/app/src/cli/services/dev/processes/setup-dev-processes.ts | Passes realExtensions into previewable extension process setup. |
| packages/app/src/cli/services/dev/processes/dev-session/dev-session.ts | Workaround to always include admin in update manifests. |
| packages/app/src/cli/services/dev/extension/server/middlewares.ts | Adds middleware to serve app assets by key/path. |
| packages/app/src/cli/services/dev/extension/server.ts | Registers the new app-assets route when asset directories exist. |
| packages/app/src/cli/services/dev/extension/payload/store.ts | Adds admin-derived app config/assets to payload and tracks asset directories/timestamps. |
| packages/app/src/cli/services/dev/extension/payload/store.test.ts | Adjusts store option mocks for new constructor usage. |
| packages/app/src/cli/services/dev/extension/payload/models.ts | Updates payload interface with new app fields. |
| packages/app/src/cli/services/dev/extension.ts | Triggers admin config/timestamp updates on extension events; refactors server setup call. |
| packages/app/src/cli/services/dev/extension.test.ts | Updates mocks/assertions for new payload store methods. |
| packages/app/src/cli/services/dev/app-events/file-watcher.ts | Watches realExtensions files (including config extensions). |
| packages/app/src/cli/services/build/steps/include-assets/generate-manifest.ts | Changes manifest generation to overwrite existing manifest.json. |
| packages/app/src/cli/services/build/steps/include-assets-step.test.ts | Updates tests to expect manifest overwrite behavior. |
| packages/app/src/cli/models/extensions/specifications/admin.ts | Adds allowed_domains to schema; exports AdminConfigType; watches static_root files. |
| packages/app/src/cli/models/extensions/specification.ts | No functional change (formatting). |
| packages/app/src/cli/models/extensions/extension-instance.ts | Uses isAppConfigExtension when deciding default dev-session watch behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function getAppAssetsMiddleware(getAppAssets: () => Record<string, string> | undefined) { | ||
| return defineEventHandler(async (event) => { | ||
| const {assetKey = '', filePath = ''} = getRouterParams(event) | ||
| const appAssets = getAppAssets() | ||
| const directory = appAssets?.[assetKey] | ||
| if (!directory) { | ||
| return sendError(event, {statusCode: 404, statusMessage: `No app assets configured for key: ${assetKey}`}) | ||
| } | ||
| return fileServerMiddleware(event, { | ||
| filePath: joinPath(directory, filePath), | ||
| }) |
There was a problem hiding this comment.
filePath comes from the URL and is joined directly with the configured directory. Because joinPath() will allow absolute segments and .., a request like /extensions/assets/staticRoot/../../../../etc/passwd (or a leading /) can escape the intended directory and expose arbitrary files. Resolve the requested path and enforce it stays within the configured directory (e.g., resolvePath(directory, filePath) + isSubpath(directory, resolved)), and reject absolute paths / traversal with a 400/403.
| // Admin extension contributes app-level config to the payload | ||
| const adminExtension = options.extensions.find((ext) => ext.type === 'admin') | ||
| if (adminExtension) { | ||
| const adminConfig = (adminExtension.configuration as AdminConfigType).admin | ||
| if (adminConfig?.allowed_domains) { | ||
| payload.app.allowed_domains = adminConfig.allowed_domains | ||
| } | ||
| if (adminConfig?.static_root) { | ||
| const assetKey = 'staticRoot' |
There was a problem hiding this comment.
This logic derives adminExtension from options.extensions, but in the previewable extensions dev server options.extensions is filtered to ext.isPreviewable (so admin is excluded). That means payload.app.allowed_domains / payload.app.assets will never be set even when an admin extension exists. Consider sourcing the admin extension from the full app extension list (or pass it separately) rather than from the previewable-only list.
| private getAllWatchedFiles(): string[] { | ||
| this.extensionWatchedFiles.clear() | ||
|
|
||
| const extensionResults = this.app.nonConfigExtensions.map((extension) => ({ | ||
| const extensionResults = this.app.realExtensions.map((extension) => ({ | ||
| extension, | ||
| watchedFiles: extension.watchedFiles(), | ||
| })) |
There was a problem hiding this comment.
getAllWatchedFiles() now includes realExtensions, but extensionPaths (used to decide whether to emit delayed file_deleted events) is still initialized from nonConfigExtensions in updateApp(). For configuration extensions like admin, this causes delete events to be dropped (extensionPaths.includes(extensionPath) is false), so asset deletions won’t trigger extension events/timestamp updates. Consider tracking extensionPaths for realExtensions as well to keep event emission consistent with the new watch set.
| // WORKAROUND. This is a temporary fix because `admin` is not compatible with inheritedUids in Core. | ||
| // It needs to be included in the manifest always if present in the app. | ||
| if (appEvent.app.allExtensions.some((ext) => ext.type === 'admin')) { | ||
| updatedUids.push('admin') | ||
| } |
There was a problem hiding this comment.
This workaround unconditionally pushes 'admin' into updatedUids whenever an admin extension exists. Since updatedUids is later used to filter the manifest modules for UPDATE sessions, this will force the admin module into every update (potentially increasing bundle size/upload time) and may add duplicates. At minimum, de-dupe updatedUids (e.g., use a Set) and add a tracking reference (issue/PR) so the workaround can be removed once Core supports inheritedUids for admin.
9dd6439 to
8cb333a
Compare
…Config Co-authored-by: Claude Code <claude-code@anthropic.com>
8cb333a to
06b860d
Compare
| url: string | ||
| mobileUrl: string | ||
| title: string | ||
| allowed_domains?: string[] |
There was a problem hiding this comment.
Can we make this allowedDomains so it's consistent with the other fields which are camel cased?

WHY are these changes introduced?
Adds support for serving app-level assets (such as admin static files) through the development server to enable proper asset handling during local development.
WHAT is this pull request doing?
appAssetsConfiginterface to extension specifications to define asset configurationappAssetsConfigfor admin extensions to handlestatic_rootconfiguration/extensions/assets/:assetKey/endpointsmanifest.jsonfiles instead of throwing errorsHow to test your changes?
static_rootconfiguredshopify app devChecklist
pnpm changeset add